fix(quantization): accept QuantizeAlgorithmConfig in get_modelike_from_algo_cfg#1514
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR normalizes algorithm-config inputs that are Mappings or QuantizeAlgorithmConfig instances to plain dicts in ChangesQuantizeAlgorithmConfig instance acceptance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| algo_cfg = algo_cfg.model_dump() | ||
| if algo_cfg is None or isinstance(algo_cfg, str): | ||
| algo_name, algo_cfg = algo_cfg, {} | ||
| elif isinstance(algo_cfg, dict): |
There was a problem hiding this comment.
Thanks for helping fix this bug. Let's instead change this to
| elif isinstance(algo_cfg, dict): | |
| elif isinstance(algo_cfg, Mapping): |
QuantizeAlgorithmConfig is a MutableMapping.
There was a problem hiding this comment.
Thanks for the review, and good call. I'll rebase onto current main.
One follow-on: with that change the resolved config is passed through as a Mapping rather than a fresh dict, so I'll update the isinstance(..., dict) assertion in the new unit test to match.
There was a problem hiding this comment.
OK I was thinking of using Mapping so we can get rid of the dict conversion, but, I guess need more changes across the context. Nevermind.
| pass | ||
|
|
||
|
|
||
| def test_get_modelike_from_algo_cfg_accepts_config_instance(): |
There was a problem hiding this comment.
Tests look good. Thanks for adding them.
There was a problem hiding this comment.
Thanks! I'll keep them in the revised version, with the dict assertion updated to reflect the Mapping pass-through.
…m_algo_cfg get_modelike_from_algo_cfg raised ValueError for a QuantizeAlgorithmConfig instance even though QuantizeAlgoCfgType lists it as a valid type. Since ModeloptBaseConfig is a Mapping, widen the type cascade's dict check to Mapping and materialize the resolved config with dict(). Add unit and end-to-end regression tests plus a CHANGELOG entry. Fixes NVIDIA#201 Signed-off-by: javierdejesusda <javier.dejesusj9@gmail.com>
a474a71 to
7b5e4c8
Compare
What does this PR do?
Type of change: Bug fix
Fixes #201 -
get_modelike_from_algo_cfgrejectedQuantizeAlgorithmConfiginstances withValueError, despite thealgorithmfield's type annotation allowing them.Root cause:
QuantizeAlgoCfgType(modelopt/torch/quantization/config.py) explicitly includesQuantizeAlgorithmConfig, andQuantizeConfig.algorithmis typed with it. Butget_modelike_from_algo_cfgonly matchedlist/None/str/dictand fell through toraise ValueError(f"Invalid config type: {type(algo_cfg)}")for a config instance:Fix:
ModeloptBaseConfigis acollections.abc.MutableMapping, so the type cascade'sdictcheck is widened toMapping, and the matched config is materialized withdict(...)so the resolved config keeps itsConfigDict(dict[str, Any]) type:The change is a strict no-op for the previously supported
str/dict/None/listinputs (a plaindictis aMapping); only the path that previously raised changes.Usage
Testing
Added two regression tests:
tests/unit/torch/quantization/test_mode.py::test_get_modelike_from_algo_cfg_accepts_config_instance- a single config instance and a list of config instances (resolved mode names, dict round-trip).tests/unit/torch/quantization/test_quantize_cpu.py::test_quantize_accepts_algo_config_instance_end_to_end- the issue's repro through the fullmtq.quantize/ calibrate flow.pytest tests/unit/torch/quantization/test_mode.py::test_get_modelike_from_algo_cfg_accepts_config_instance \ tests/unit/torch/quantization/test_quantize_cpu.py::test_quantize_accepts_algo_config_instance_end_to_endWithout the fix both tests fail with the exact
ValueError: Invalid config type; with it they pass. The fulltest_mode.pyandtest_quantize_cpu.pysuites pass locally, andruff check/ruff format --checkare clean on the changed files.Before your PR is "Ready for review"
Make sure you read and follow the Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices for contributors.
CONTRIBUTING.md: N/AAdditional Information
Closes #201. Bug confirmed by @realAsma on the issue thread. The fix uses the
Mapping-based approach suggested by @shengliangxu in review (ModeloptBaseConfigbecame aMutableMappingin #1405). Change is +41/-4 acrossmodelopt/torch/quantization/mode.py, the two test files, andCHANGELOG.rst.Summary by CodeRabbit
Bug Fixes
Tests
Documentation